fix: re-read settings when CACHEKIT_MASTER_KEY appears + correct missing-key env var name#200
Merged
Merged
Conversation
…ing-key env var name #195: get_settings() cached the config once; if first built before CACHEKIT_MASTER_KEY was in the environment (e.g. an import-time cache decorator before secrets load), it froze master_key=None forever — encryption then silently never activated. get_settings() now re-reads once the key appears (under the existing lock; idempotent, no churn after). #194: the missing-key error and a docstring named REDIS_CACHE_MASTER_KEY, which is never read; corrected both to CACHEKIT_MASTER_KEY (the only honored var), matching config/nested.py. Closes #194. Closes #195.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCorrects ChangesMaster key env var fix and singleton self-heal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cachekit/config/singleton.py`:
- Around line 49-60: The code has a race condition where another thread calling
reset_settings() between the unlocked check on line 55 and the locked check on
line 59 could set _settings_instance to None, causing an AttributeError when
trying to access _settings_instance.master_key. Capture _settings_instance into
a local variable before the initial unlocked check, then use that cached
reference throughout the condition to prevent the instance from becoming None
between the two checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd606741-8805-4d1f-81ab-56cd841ff3cf
📒 Files selected for processing (4)
src/cachekit/cache_handler.pysrc/cachekit/config/singleton.pysrc/cachekit/serializers/encryption_wrapper.pytests/unit/test_encryption_config_resolution.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ncurrent-reset AttributeError) Addresses CodeRabbit on #200: the #195 self-heal dereferenced _settings_instance.master_key in the unlocked fast path after a non-None check. A concurrent reset_settings() between the two could null the global and AttributeError (or return None). Snapshot the global to a local for the unlocked checks; re-read it under the lock (rebuilding if a peer reset or it is still keyless).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two encryption-config correctness fixes.
Closes #195. Closes #194.
#195 — keyless settings singleton froze permanently
get_settings()builtCachekitConfigonce and cached it process-wide. If the first build happened beforeCACHEKIT_MASTER_KEYwas in the environment — e.g. an import-time@cache/@cache.iodecorator evaluated before the app loads its secrets (python-dotenv, cloud secret managers) — it frozemaster_key=Noneforever. Setting the env var afterward had no effect: with auto-encryption that means data is cached plaintext though a key is present, and with explicit@cache.secureit surfaces as #194's confusing "master key required".Fix:
get_settings()now re-reads once the key appears (master_key is NoneandCACHEKIT_MASTER_KEYnow set → rebuild, under the existing lock). Idempotent — after the rebuildmaster_keyis set, so it never fires again (no per-call churn).#194 — missing-key error named a nonexistent env var
The error and a
master_keydocstring told users to setREDIS_CACHE_MASTER_KEY, which is never read; the only honored variable isCACHEKIT_MASTER_KEY. Corrected both strings (encryption_wrapper.py+cache_handler.py), matching the already-correct sibling message inconfig/nested.py.Scope / known residual
The self-heal fixes the singleton and any handler built after the key loads. A
CacheSerializationHandleris built eagerly when@cacheis applied (decoration/import) and freezes its encryption decision at__init__; a handler frozen keyless at import is not retroactively unfrozen by this change. Fully closing that (auto-encryption + import-time decorator + secrets-after-import) needs lazy encryption resolution in the handler — invasive, security-critical, deferred to its own PR. Note@cache.secure(explicit) already fails loud in that window, so only the auto-encryption path is silent.Tests
New
tests/unit/test_encryption_config_resolution.py: get_settings() re-reads when the key appears (the #195 repro), stays stable once loaded, and the missing-key error namesCACHEKIT_MASTER_KEY. Full suite: 1866 passed, basedpyright 0 errors.Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation & Error Handling
CACHEKIT_MASTER_KEYwhen the master key is missing.